Python: omit failed Foundry turns from conversation chat history - #7637
Conversation
|
@microsoft-github-policy-service agree |
|
Unfortunately this doesn't fix the issue, as the problem is that the messages get saved to the chat history, not the session. |
|
You're right — the first commit skipped the wrong store. The agentserver response provider was persisting input items for failed turns, so the next request on the same conversation replayed them via chat history (get_history()), not the MAF session. 24ebe8b wraps the response store so failed turns persist without input items (including the in_progress → failed update path). A follow-up request on the same conversation no longer sees the bad function_call_output. Tests cover both the store wrapper and the HTTP conversation repro from the issue. |
9c43deb to
4b6b97e
Compare
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
|
/review |
|
Shikhar Goel (@sgoel2be24-cyber) please fix the merge conflict, and there is a update coming to the way we handle storage in #7997 I suggest updating this after that one merged |
4b6b97e to
79aa6e3
Compare
|
Thanks Eduard van Valkenburg (@eavanvalkenburg). The merge conflict is fixed: rebased onto main (1a51fdd) and pushed 79aa6e3, including the prepared docstring and concrete-type cleanup. Foundry Hosting validation passes: 276 unit tests, 90% package coverage, Ruff, strict Pyright, and all five test type-checkers. The two cleanup threads are resolved; the streaming/storage thread remains open. I will revisit that design after #7997 merges, as suggested. |
79aa6e3 to
8213cc6
Compare
|
Follow-up: #7997 merged while I was validating the earlier update. I have now rebased onto it (1f7f4b3) and pushed 8213cc6. Failed-input filtering is scoped to regular agents using AgentServer history; agent-owned history retains normal protocol storage and bypasses buffering. Added regression coverage for both modes and updated the resilient-store guard test for the new constructor validation order. All 294 Foundry Hosting unit tests pass, with 91% coverage; Ruff, strict Pyright, and all five test type-checkers pass. The docstring/type cleanup remains complete. The remaining default-mode buffering/API question is documented in the open review thread. |
|
/review |
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (3 commit(s)): 3fb9d59225fd, 5b4281fae1fc, 8213cc685bc4
Model: gpt-5.6-sol-fast
Overview
The change correctly lets AgentServer resolve its configured store before wrapping it and adds end-to-end coverage that failed synchronous conversation turns are not replayed. The history-source guards and successful marker-consumption path are well tested. Two residual defects remain: failed-ID state is shared across tenant contexts and can survive persistence failures, while the provider wrapper also removes inputs from standalone failed responses that cannot affect conversation history.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (1 high, 1 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/foundry_hosting/agent_framework_foundry_hosting/_responses.py
Signed-off-by: sgoel2be24-cyber <223222024+sgoel2be24-cyber@users.noreply.github.com>
8213cc6 to
aedebb2
Compare
Signed-off-by: Shikhar Goel <223222024+sgoel2be24-cyber@users.noreply.github.com>
Motivation & Context
When a hosted Foundry agent is used with
conversation_id, a failed turn still stored its input items on the agentserver response/conversation store. The next request on that conversation replayed them viaget_history(). Azure OpenAI does not keep failed input on the conversation.Fixes #7630
Description & Review Guide
ResponsesAgentServerHostresolve and validate its configured/default response store before wrapping it, preserving hosted and local persistence defaults plus the resilient-background guard.python/packages/foundry_hosting/tests/test_responses.pycover the HTTP conversation repro, provider behavior, default persistent storage, and the resilience guard.Please focus on: failed synchronous turns stay out of
get_history(), successful turns still persist, non-conversation runs are unchanged, and the base host's storage behavior is preserved.Related Issue
Fixes #7630
Contribution Checklist
Assistance: drafted with an AI coding agent and reviewed before opening.